feat(app-server): add history_mode to thread - #29927
Conversation
d058227 to
311883f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 311883fa89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
311883f to
c953f53
Compare
8ef00eb to
f3655c0
Compare
| /// | ||
| /// The default is legacy so existing stores stay compatible. Stores whose durable contract is | ||
| /// already paginated should override this instead of relying on core to infer storage behavior. | ||
| fn default_history_mode(&self) -> ThreadHistoryMode { |
There was a problem hiding this comment.
should this be a top level read attr, or should it be a property of thread metadata?
There was a problem hiding this comment.
I think top-level is right for now. This is the store fallback for histories that do not already carry persisted thread metadata, mainly so a remote paginated store can opt into its durable contract. Persisted threads still carry the actual mode in SessionMeta / thread metadata.
wiltzius-openai
left a comment
There was a problem hiding this comment.
some detail q's but looks good
| }, | ||
| selected_capability_roots, | ||
| memory_mode: (!config.generate_memories()).then_some("disabled".to_string()), | ||
| history_mode: Default::default(), |
There was a problem hiding this comment.
We still hardcode SessionMeta.history_mode = legacy when creating the rollout
Since CreateThreadParams.history_mode never reaches RolloutRecorder, the JSONL compatibility floor isn’t actually established yet. enabling paginated later would still write legacy-looking rollouts
There was a problem hiding this comment.
or am I missing something?
There was a problem hiding this comment.
oh yeah this PR doesn't actually support history_mode = "paginated" yet (we throw a json-rpc error), so codex decided to hardcode this instead of plumbing it through properly. I'll plumb it through properly now though so it's not confusing
9523797 to
4d6ed79
Compare
4d6ed79 to
c80f9bc
Compare
Description
This PR adds a new
historyMode = "legacy" | "paginated"toThread. This will be stored inSessionMetain the JSONL rollout file and as a new column in the SQLite thread_metadata table, and exposed onthread/startand on theThreadobject in app-server.What changed
ThreadHistoryModewithlegacyandpaginated, defaulting old and new SessionMeta tolegacy.history_modethrough core session config, ThreadStore stored metadata, local/in-memory stores, rollout metadata extraction, and the existing SQLitethreadstable.historyModeto app-server v2Threadandthread/start.load_history, live resume, and create paths.Compatibility floor
Because users may be running various versions of Codex binaries on the same machine (TUI, Codex App, etc.), we will need to establish a compatibility floor for upcoming paginated threads, which will change how thread storage reads and writes work.
The overall plan here:
The important behavior change is fail-closed handling for a binary that encounters a persisted
paginatedthread before it knows how to fully support paginated history. In app-server, if a thread ispaginated, we will:thread/listandthread/read(includeTurns=false), so clients can still see the thread and inspect itshistoryModethread/read(includeTurns=true)andthread/resumewith an unsupported JSON-RPC errorhistoryModeaslegacyUnder the hood, the ThreadStore layer also rejects legacy operations that would need to load or replay the full thread history for a paginated thread. That gives us the behavior we want for Release N: future paginated threads are visible, but this binary fails closed instead of trying to operate on them as if they were legacy threads.